Feature: Convert menus to JSON #195

colinmurphy · 2024-12-10T13:44:26Z

Overview:

I converted the header and footer menus into static menus using a JSON array.
The purpose of this task was firstly to remove the loading text on the initial page load and also to add SSR.

One tradeoff was around having 3 menus or just one menu for the footer which we could filter. I considered 3 menus but I found that the code was cleaner with one menu and I was able to filter each section.

QA

Manually checked that both header and footer links matched what was on the current development site
Manually opening all links to make sure they all worked

Screenshots below

headless-platform-by-wp-engine · 2024-12-10T13:49:43Z

Check out the recent updates to your Headless Platform preview environment:

App	Environment	URL	Build
faustjs.org	preview-env-feature/convert-menus-to-json	https://hu…wered.com	✅ (logs)

Learn more about preview environments in our documentation.

kellenmace

@colinmurphy So, you executed on the new JSON file approach for this feature beautifully, but now seeing it implemented, I'm wondering if maybe this approach is unnecessarily complex. 😅

What if we just hardcode the menu links directly in the primary-nav and footer files?

The menus are not used anywhere else in the app, so there's no real reason why they need to live in a separate menus.js file. And with the current approach, we have to jump through a bunch of hoops:

Add unique ids for the menu items so that when we map over them we can pass those in as the key prop to appease React
Add a section property for every object to keep track of which column it's supposed to be in.
Do filtering in the footer file to sort the menu items in three lists
Map over the menu items and render the nav links

If we hardcode the nav links directly in the primary-nav and footer files, we could wipe out all of those steps and simplify this code significantly. We'd have to repeat the className class lists for each column and nav link, but I don't have a problem with that.

What do you think of this simpler hardcoding approach, @colinmurphy & @Fran-A-Dev?

kellenmace · 2024-12-10T15:23:51Z

src/constants/menus.js

+		target: "_blank",
+	},
+	{
+		id: "dicord",


Typo here– missing an "s". Not that it matters too much, since this is just a unique ID that end users never see :)

Thanks Kellen :)

kellenmace · 2024-12-10T15:27:50Z

src/constants/menus.js

 export const PRIMARY_LOCATION = "PRIMARY";
 export const FOOTER_LOCATION = "FOOTER_1";
+
+export const HeaderMenu = [


I would name this using camelCase instead of PascalCase here. By convention, PascalCase is typically used for classes and React components and camelCase is used for other variables like this array. When I first glanced at this PR and saw HeaderMenu, my first thought was that it was a React component that was being imported.

Thanks Kellen, That is really useful feedback. I wasn't aware of the different conventions for React. Thank you.

ya same with the HeaderMenu in my mind being a React component.

kellenmace · 2024-12-10T15:30:25Z

src/components/footer.jsx


 	return columns.map((column, index) => {
 		if (!column || column.length === 0) {
 			return; // Skip rendering if no menu items are found


Per this section of the React docs (https://react.dev/learn/conditional-rendering#conditionally-returning-nothing-with-null), the recommended way to render nothing is to return null. So this line should do that.

Thanks Kellen, That is good to know.

Thanks Kellen for the really good feedback. I can definitely see where your coming from and I agree with your approach too. There is definitely too much overhead for some static links which are not going to be re-used elsewhere. We could also probably set a variable for the class names to try and reduce repetition too.

Fran-A-Dev · 2024-12-10T18:23:36Z

@colinmurphy So, you executed on the new JSON file approach for this feature beautifully, but now seeing it implemented, I'm wondering if maybe this approach is unnecessarily complex. 😅

What if we just hardcode the menu links directly in the primary-nav and footer files?

The menus are not used anywhere else in the app, so there's no real reason why they need to live in a separate menus.js file. And with the current approach, we have to jump through a bunch of hoops:

Add unique ids for the menu items so that when we map over them we can pass those in as the key prop to appease React

Add a section property for every object to keep track of which column it's supposed to be in.

Do filtering in the footer file to sort the menu items in three lists

Map over the menu items and render the nav links

If we hardcode the nav links directly in the primary-nav and footer files, we could wipe out all of those steps and simplify this code significantly. We'd have to repeat the className class lists for each column and nav link, but I don't have a problem with that.

What do you think of this simpler hardcoding approach, @colinmurphy & @Fran-A-Dev?

Agreed @kellenmace , this is how I would do it myself as well. Keeping this as simple as possible. I tend to hardcode things whenever possible and if it is the best approach to keep it simple.

headless-platform-by-wp-engine · 2024-12-10T18:29:03Z

Check out the recent updates to your Headless Platform preview environment:

App	Environment	URL	Build
faustjs.org	preview-env-feature/convert-menus-to-json	https://hu…wered.com	✅ (logs)

Learn more about preview environments in our documentation.

colinmurphy · 2024-12-10T18:30:32Z

@kellenmace @Fran-A-Dev

The PR has now been updated as per code review (thanks for the feedback) and the links are now hardcoded.

moonmeister · 2024-12-10T21:34:35Z

I disagree completely with Kellen and Fran but 🤷‍♂️. The first system was over engineered but directly replaced the JS API without engineering the HTML/CSS. You didn't need the id or targut fields. And sections could have been achieved by simply exporting multiple objects instead of doing the complex filtering.

Kellen can make the call but I would have refined the previous idea instead of re-engineering something that has to be undone when we revert to WP menus.

moonmeister · 2024-12-10T21:35:53Z

src/components/footer.jsx

+							className={linkClass}
+							href="https://www.npmjs.com/package/@faustwp/cli"
+							noDefaultStyles
+							target="_blank"


This is handled by the component.

moonmeister · 2024-12-10T21:37:21Z

src/components/footer.jsx

+					</li>
+				</ul>
+			</div>
+			<div className={columnClass} key="community">


Keys are unneeded if we're not mapping over an array.

@colinmurphy @kellenmace @moonmeister I am for whatever ya'll decide is best! Stoked

colinmurphy · 2024-12-11T10:34:03Z

Thanks Alex for the feedback. I won't do anything further here until Kellen reviews. Happy to do either approach. In hindsight the filter approach was a bit too complex and going with 3 objects instead would of been the better approach.

Fran-A-Dev · 2024-12-11T14:04:53Z

Thanks Alex for the feedback. I won't do anything further here until Kellen reviews. Happy to do either approach. In hindsight the filter approach was a bit too complex and going with 3 objects instead would of been the better approach.

headless-platform-by-wp-engine · 2024-12-11T17:22:03Z

Check out the recent updates to your Headless Platform preview environment:

App	Environment	URL	Build
faustjs.org	preview-env-feature/convert-menus-to-json	https://hj…wered.com	✅ (logs)

Learn more about preview environments in our documentation.

colinmurphy · 2024-12-11T18:13:05Z

@kellenmace @Fran-A-Dev @moonmeister This is now ready to be reviewed again. As per call I removed the key and target elements. I also double checked that privacy policy link does not open a new window too.

headless-platform-by-wp-engine · 2024-12-11T18:16:40Z

Check out the recent updates to your Headless Platform preview environment:

App	Environment	URL	Build
faustjs.org	preview-env-feature/convert-menus-to-json	https://hj…wered.com	✅ (logs)

Learn more about preview environments in our documentation.

headless-platform-by-wp-engine · 2024-12-11T18:42:03Z

Check out the recent updates to your Headless Platform preview environment:

App	Environment	URL	Build
faustjs.org	preview-env-feature/convert-menus-to-json	https://hj…wered.com	✅ (logs)

Learn more about preview environments in our documentation.

kellenmace

This last iteration looks great to me. Thanks @colinmurphy! 👍🏼

colinmurphy added 2 commits December 10, 2024 10:56

Replaced primary menu

c6682ae

Added footer menus

d801d87

colinmurphy changed the base branch from canary to toolkit December 10, 2024 13:46

colinmurphy requested review from Fran-A-Dev, kellenmace and moonmeister December 10, 2024 13:46

kellenmace suggested changes Dec 10, 2024

View reviewed changes

colinmurphy added 2 commits December 10, 2024 17:43

Hardcoded header links

8f87253

Hardcoded footer menus

e388fea

moonmeister requested changes Dec 10, 2024

View reviewed changes

Fran-A-Dev closed this Dec 11, 2024

kellenmace reopened this Dec 11, 2024

Removed keys and target _blank as per code review

e2e7b89

colinmurphy marked this pull request as ready for review December 11, 2024 18:15

Fixed path for discord

9a7e6dd

martinmcdonnell assigned colinmurphy Dec 12, 2024

kellenmace approved these changes Dec 12, 2024

View reviewed changes

kellenmace merged commit b5ffb44 into toolkit Dec 12, 2024
3 checks passed

kellenmace deleted the feature/convert-menus-to-json branch December 12, 2024 18:59

Feature: Convert menus to JSON #195

Feature: Convert menus to JSON #195

Conversation

colinmurphy commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

headless-platform-by-wp-engine bot commented Dec 10, 2024

Uh oh!

kellenmace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fran-A-Dev commented Dec 10, 2024

Uh oh!

headless-platform-by-wp-engine bot commented Dec 10, 2024

Uh oh!

colinmurphy commented Dec 10, 2024

Uh oh!

moonmeister commented Dec 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colinmurphy commented Dec 11, 2024

Uh oh!

Fran-A-Dev commented Dec 11, 2024

Uh oh!

headless-platform-by-wp-engine bot commented Dec 11, 2024

Uh oh!

colinmurphy commented Dec 11, 2024

Uh oh!

headless-platform-by-wp-engine bot commented Dec 11, 2024

Uh oh!

headless-platform-by-wp-engine bot commented Dec 11, 2024

Uh oh!

kellenmace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

colinmurphy commented Dec 10, 2024 •

edited

Loading